Ensure socket path is < 104 characters when STATE_PATH is set. #4909

belimawr · 2024-06-11T17:29:09Z

What does this PR do?

It ensures the statePath set by the container command will always generate an Unix socket path that is smaller than 108 characters. This ensures the Unix socket can be created and the Elastic-Agent always successfully starts.

Why is it important?

Unix socket paths need to be less than 104 characters long, if they're not Elastic-Agent will fail to create a socket and will not start. When it fails it logs those messages:

{"log.level":"error","@timestamp":"2024-06-10T21:34:44.818Z","log.logger":"control","log.origin":{"function":"github.com/elastic/elastic-agent/pkg/control/v2/server.(*Server).Start","file.name":"server/server.go","file.line":85},"message":"unable to create listener: listen unix /tmp/TestContainerCMDEventToStderr1447633574/001/elastic-agent-8.15.0-SNAPSHOT-linux-x86_64/data/elastic-agent.sock: bind: invalid argument","log":{"source":"elastic-agent"},"ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2024-06-10T21:34:44.818Z","log.origin":{"function":"github.com/elastic/elastic-agent/internal/pkg/agent/cmd.logReturn","file.name":"cmd/run.go","file.line":147},"message":"listen unix /tmp/TestContainerCMDEventToStderr1447633574/001/elastic-agent-8.15.0-SNAPSHOT-linux-x86_64/data/elastic-agent.sock: bind: invalid argument","log":{"source":"elastic-agent"},"ecs.version":"1.6.0"}

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in ./changelog/fragments using the changelog tool
I have added an integration test or an E2E test

Disruptive User Impact

The Elastic-Agent container command will not always respect the STATE_PATH environment variable. In cases where it would lead to a Unix socket path too long, which prevents the Elastic-Agent from starting, the default path /usr/share/elastic-agent/state is used.

How to test this PR locally

Get a Stack + Fleet deployed
Set the following environment variables according to your deployment:
- FLEET_ENROLL
- FLEET_URL
- FLEET_ENROLLMENT_TOKEN
Set STATE_PATH to be longer than 100 characters
Run the Elastic-Agent container command
Assert:
i. The Elastic-Agent runs and can communicate with Fleet
ii. The state path is the default path :/usr/share/elastic-agent/state

Another option is to run the integration test TestContainerCMDWithAVeryLongStatePath from testing/integration/container_cmd_test.go

~~## Related issues~~

Questions to ask yourself

How are we going to support this in production?
How are we going to measure its adoption?
How are we going to debug this?
What are the metrics I should take care of?
...

mergify · 2024-06-11T17:29:52Z

This pull request does not have a backport label. Could you fix it @belimawr? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

elasticmachine · 2024-06-12T21:08:00Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

belimawr · 2024-06-13T12:03:53Z

Quality Gate failed

Failed conditions 0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

The changes are covered by integration tests.

This commit fixes the implementation on Windows and puts the container integration tests in the container group again. The test for long state paths will create files outside of the test directory. It also cleans up the default folder used as state path.

internal/pkg/agent/cmd/container.go

michel-laterman

Just have a couple of nitpicks

Also if the STATE_PATH is too long and the default is used, would that be clear in diagnostics?

internal/pkg/agent/cmd/container.go

testing/integration/container_cmd_test.go

pchila

LGTM, no major blockers, Just a nitpick on a direct cast for the agent cmd in test.

While I understand that this may not work for everyone, I feel that the state path should be mounted on a well known location as a local persistent volume or some other volume stored on the host that should have the same lifetime as the deployment: having the STATE_PATH env var does not feel very container/cloud-native..

testing/integration/container_cmd_test.go

Co-authored-by: Michel Laterman <[email protected]>

cmacknz · 2024-06-18T19:57:17Z

testing/integration/container_cmd_test.go

+			expectedStatePath:  "/tmp/tttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt",
+			expectedSocketPath: "/tmp/elastic-agent/oKUUJbxrLlGSh3z6wZWYleLeMuUN4P0_.sock",
+		},
+		"long path": { // Path too long to create a unix socket, it will use /tmp/elastic-agent


Aren't most of these test cases just tests of the SocketURLWithFallback() function? How long does this new set of tests take to run?

Do you get the same effective test coverage if you update the single existing container command to use a path longer than 107 characters as the exception path?

Aren't most of these test cases just tests of the SocketURLWithFallback() function? How long does this new set of tests take to run?

No, they're testing the https://github.com/elastic/elastic-agent/blob/a6d91b029ae8c0b8ccf7bb391c8ac304fe1e9368/internal/pkg/agent/cmd/container.go#L773-L837 function and how it decides to create the socket path. The fact that it calls SocketURLWithFallback is an implementation detail. It also needs to create some folders:

elastic-agent/internal/pkg/agent/cmd/container.go

Lines 784 to 788 in a6d91b0

if _, err := os.Stat(configPath); errors.Is(err, fs.ErrNotExist) {

if err := os.MkdirAll(configPath, 0755); err != nil {

return fmt.Errorf("cannot create folders for config path '%s': %w", configPath, err)

}

}

elastic-agent/internal/pkg/agent/cmd/container.go

Lines 794 to 796 in a6d91b0

if err := os.MkdirAll(topPath, 0755); err != nil {

return fmt.Errorf("preparing STATE_PATH(%s) failed: %w", statePath, err)

}

All of this gets tested by the integration test.

Maybe we can remove one test case: 107 characters path as it essentially is the same as small path.

cmacknz · 2024-06-18T20:16:30Z

Once this compiles, make sure the elastic-agent status command still works inside the container.

belimawr · 2024-06-18T21:04:01Z

Once this compiles, make sure the elastic-agent status command still works inside the container.

It works:

% k exec -it elastic-agent-vspdt -- /bin/bash
�]0;root@kind-control-plane: /usr/share/elastic-agentroot@kind-control-plane:/usr/share/elastic-agent# elastic-agent status
┌─ fleet
│  └─ status: (HEALTHY) Connected
└─ elastic-agent
   └─ status: (HEALTHY) Running
�]0;root@kind-control-plane: /usr/share/elastic-agentroot@kind-control-plane:/usr/share/elastic-agent#

elastic-sonarqube · 2024-06-20T19:37:23Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

jlind23 · 2024-06-21T07:00:57Z

Everything works except he sonar unit code coverage which is superseded by the integration tests hence force merging this.

mergify bot assigned belimawr Jun 11, 2024

mergify bot added the backport-skip label Jun 11, 2024

belimawr force-pushed the fix-container-socket-path branch from 3859da7 to 8679507 Compare June 11, 2024 17:33

ycombinator added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Jun 11, 2024

belimawr marked this pull request as ready for review June 12, 2024 21:07

belimawr requested a review from a team as a code owner June 12, 2024 21:07

belimawr requested review from andrzej-stencel and michel-laterman June 12, 2024 21:07

belimawr changed the title ~~Ensure socket path when STATE_PATH is set is < 104 characters long~~ Ensure socket path is <108 characters when STATE_PATH is set. Jun 13, 2024

belimawr changed the title ~~Ensure socket path is <108 characters when STATE_PATH is set.~~ Ensure socket path is < 108 characters when STATE_PATH is set. Jun 13, 2024

belimawr added 5 commits June 13, 2024 10:43

Ensure socket path when STATE_PATH is set is < 104 characters long

151930f

Fix corner cases and improve tests

845fff3

Add changelog

916a96a

Fix lint warning

ff5d781

belimawr mentioned this pull request Jun 13, 2024

Update event logger configuration via Fleet and environment variable (container command) #4932

Merged

7 tasks

cmacknz reviewed Jun 13, 2024

View reviewed changes

internal/pkg/agent/cmd/container.go Outdated Show resolved Hide resolved

cmacknz reviewed Jun 13, 2024

View reviewed changes

internal/pkg/agent/cmd/container.go Outdated Show resolved Hide resolved

belimawr added 3 commits June 17, 2024 12:18

Only change socket path

ccf9439

Update tests to new approach

9d13546

remove un-used function

e46948b

belimawr force-pushed the fix-container-socket-path branch from 6dcc963 to e46948b Compare June 17, 2024 21:36

belimawr requested a review from cmacknz June 17, 2024 21:40

michel-laterman reviewed Jun 18, 2024

View reviewed changes

internal/pkg/agent/cmd/container.go Outdated Show resolved Hide resolved

testing/integration/container_cmd_test.go Outdated Show resolved Hide resolved

belimawr changed the title ~~Ensure socket path is < 108 characters when STATE_PATH is set.~~ Ensure socket path is < 104 characters when STATE_PATH is set. Jun 18, 2024

pchila approved these changes Jun 18, 2024

View reviewed changes

testing/integration/container_cmd_test.go Show resolved Hide resolved

Update internal/pkg/agent/cmd/container.go

a6d91b0

Co-authored-by: Michel Laterman <[email protected]>

belimawr requested a review from michel-laterman June 18, 2024 15:37

cmacknz reviewed Jun 18, 2024

View reviewed changes

belimawr and others added 2 commits June 20, 2024 08:31

fix build and PR suggestions

f611c40

Merge branch 'main' into fix-container-socket-path

2e79fff

jlind23 merged commit 8a30900 into elastic:main Jun 21, 2024
12 of 13 checks passed

mergify bot mentioned this pull request Jul 10, 2024

[8.15](backport #4932) Update event logger configuration via Fleet and environment variable (container command) #5109

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure socket path is < 104 characters when STATE_PATH is set. #4909

Ensure socket path is < 104 characters when STATE_PATH is set. #4909

belimawr commented Jun 11, 2024 •

edited

Loading

mergify bot commented Jun 11, 2024

elasticmachine commented Jun 12, 2024

belimawr commented Jun 13, 2024

Quality Gate failed

michel-laterman left a comment

pchila left a comment

cmacknz Jun 18, 2024

belimawr Jun 18, 2024

cmacknz commented Jun 18, 2024

belimawr commented Jun 18, 2024

elastic-sonarqube bot commented Jun 20, 2024

jlind23 commented Jun 21, 2024

	if _, err := os.Stat(configPath); errors.Is(err, fs.ErrNotExist) {
	if err := os.MkdirAll(configPath, 0755); err != nil {
	return fmt.Errorf("cannot create folders for config path '%s': %w", configPath, err)
	}
	}

	if err := os.MkdirAll(topPath, 0755); err != nil {
	return fmt.Errorf("preparing STATE_PATH(%s) failed: %w", statePath, err)
	}

Ensure socket path is < 104 characters when STATE_PATH is set. #4909

Ensure socket path is < 104 characters when STATE_PATH is set. #4909

Conversation

belimawr commented Jun 11, 2024 • edited Loading

What does this PR do?

Why is it important?

Checklist

Disruptive User Impact

How to test this PR locally

Questions to ask yourself

mergify bot commented Jun 11, 2024

elasticmachine commented Jun 12, 2024

belimawr commented Jun 13, 2024

Quality Gate failed

michel-laterman left a comment

Choose a reason for hiding this comment

pchila left a comment

Choose a reason for hiding this comment

cmacknz Jun 18, 2024

Choose a reason for hiding this comment

belimawr Jun 18, 2024

Choose a reason for hiding this comment

cmacknz commented Jun 18, 2024

belimawr commented Jun 18, 2024

elastic-sonarqube bot commented Jun 20, 2024

Quality Gate failed

jlind23 commented Jun 21, 2024

belimawr commented Jun 11, 2024 •

edited

Loading